Future::boxed and Stream::boxed should prevent double boxing #512

stepancheg · 2017-06-11T00:02:29Z

Fixes #511

travis-ci also runs tests on rust 1.10. In rust 1.10 TypeId::of requires Reflect trait. I have not found an easy way to work around this limitation, so I decided to make boxed prevent double boxing only starting from rust 1.18 (which is latest stable at the moment). Some older version between 1.10 and 1.18 could be chosen for switch, but because travis-ci does not test these versions, I think it is reasonable to enable this behavior only from 1.18.

sinkuu · 2017-06-11T12:24:29Z

Reflect bound was removed in 1.13 (according to this comment). Linux distros usually provides old versions of Rust, so including them may profit some.

3Hren · 2017-06-11T14:42:14Z

src/stream/mod.rs

@@ -250,7 +250,40 @@ pub trait Stream {
    fn boxed(self) -> BoxStream<Self::Item, Self::Error>
        where Self: Sized + Send + 'static,
    {
-        ::std::boxed::Box::new(self)
+        #[cfg(rust_at_least_1_18)]
+        fn boxed<S>(stream: S) -> BoxStream<S::Item, S::Error>


This seems to be a fully copy/paste version of boxed() for Future except the result type itself. I'd rather decompose them both to a some type-parametrized function.

Here I also tried to write generic version and couldn't.

3Hren · 2017-06-11T14:43:02Z

src/future/mod.rs

+                        &mut r as *mut BoxFuture<F::Item, F::Error> as *mut u8 as *mut F,
+                        1);
+                    mem::forget(future);
+                    r


~~Can't we just return future here without unsafe magic?~~
Understood.

No, because compiler doesn't know that future can be cast to BoxFuture<...>. Moreover, even transmute doesn't work, because compiler cannot prove that type sizes are equal.

stepancheg · 2017-06-11T15:24:11Z

OK, updated the PR with slightly more generic version.

alexcrichton · 2017-06-13T03:32:35Z

Are there measureable gains from this PR? I agree that theoretically this is a plausible optimization but I'd much rather prefer the solution in https://github.com/alexcrichton/futures-rs/pull/513/files

alexcrichton · 2017-06-13T03:33:32Z

I also fear that most users aren't actually using .boxed() because they don't want Send trait objects. For example all throughout sccache this method is barely used, if at all. In that sense I'm not sure this is worth the weight of the implementation.

stepancheg · 2017-06-13T08:39:40Z

Are there measureable gains from this PR?

I've updated PR with synthetic test. Before this PR:

test boxed             ... bench:       2,087 ns/iter (+/- 420)
test boxed_boxed       ... bench:       3,106 ns/iter (+/- 577)
test boxed_boxed_boxed ... bench:       5,536 ns/iter (+/- 1,032)
test plain             ... bench:         791 ns/iter (+/- 190)

So, accidental additional unnecessary boxing adds significant overhead for simple streams.

It is hard to say what's performance impact of accidental boxing in real applications. However, the problem is that from user point of view it is easier to simply use safe boxed instead of thinking, whether each boxed call is expensive or could be avoided.

stepancheg · 2017-06-13T09:12:55Z

I also fear that most users aren't actually using .boxed() because they don't want Send trait objects. For example all throughout sccache this method is barely used, if at all. In that sense I'm not sure this is worth the weight of the implementation.

I also simply call Box::new(..), but if boxed will provide double boxing prevention I will.

About Send, probably, another function should be added like boxed_unsend until Rust type system allows writing generic functions parameterized by optional Send, simething like:

fn boxed(self) -> Box<Future<Item=Self::Item, Error=Self::Eror> + SendIfSend<Self>>
        where Self: Sized + 'static { ... }

stepancheg · 2017-06-13T09:45:50Z

Updated the PR with specialization instead of transmute.

Fixes rust-lang#511

alexcrichton · 2017-06-13T17:38:17Z

Yes I think it's obvious that this is a performance gain in microbenchmarks, but my worry is that this doesn't actually get exercised that much higher up the stack. I'm ok with the specialization approach but I would not want to land it until specialization is stable. I'm ok leaving this PR open, however.

carllerche · 2017-06-13T17:40:05Z

I would rather see boxed deprecated and eventually removed (#228). It's probably been the (or close to) number one source of confusion of people in the gitter channel.

khuey · 2017-06-17T06:55:09Z

FWIW I usually skip boxed because I don't want 'static.

alexcrichton · 2017-08-15T04:14:36Z

Currently I agree with @carllerche that we're highly likely to remove/deprecate boxed as in #228, so I'm going to close this. Thanks though for this PR!

stepancheg force-pushed the boxed branch from 3d4a5d2 to 6105a9d Compare June 11, 2017 00:02

stepancheg changed the title ~~Future::boxed and Stream::box should prevent double boxing~~ Future::boxed and Stream::boxed should prevent double boxing Jun 11, 2017

stepancheg force-pushed the boxed branch 2 times, most recently from 990539f to be4984a Compare June 11, 2017 02:29

stepancheg mentioned this pull request Jun 11, 2017

Prevent double boxing in boxed #511

Closed

3Hren mentioned this pull request Jun 11, 2017

refactor: prevent double boxing in boxed #513

Closed

3Hren suggested changes Jun 11, 2017

View reviewed changes

stepancheg force-pushed the boxed branch from be4984a to 46f49f0 Compare June 11, 2017 15:23

stepancheg force-pushed the boxed branch 3 times, most recently from 5d593ef to c1b5845 Compare June 11, 2017 15:35

Bench boxed boxed

003925f

stepancheg force-pushed the boxed branch from c1b5845 to c1fad24 Compare June 13, 2017 08:31

stepancheg force-pushed the boxed branch from c1fad24 to 7721987 Compare June 13, 2017 09:45

Future::boxed and Stream::boxed should prevent double boxing

7575886

Fixes rust-lang#511

stepancheg force-pushed the boxed branch from 7721987 to 7575886 Compare June 13, 2017 09:49

alexcrichton closed this Aug 15, 2017

djeedai mentioned this pull request May 30, 2022

Optimize out allocations as much as possible djeedai/bevy_tweening#27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Future::boxed and Stream::boxed should prevent double boxing #512

Future::boxed and Stream::boxed should prevent double boxing #512

stepancheg commented Jun 11, 2017 •

edited

Loading

sinkuu commented Jun 11, 2017

3Hren Jun 11, 2017

stepancheg Jun 11, 2017 •

edited

Loading

3Hren Jun 11, 2017 •

edited

Loading

stepancheg Jun 11, 2017

stepancheg commented Jun 11, 2017

alexcrichton commented Jun 13, 2017

alexcrichton commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

alexcrichton commented Jun 13, 2017

carllerche commented Jun 13, 2017

khuey commented Jun 17, 2017

alexcrichton commented Aug 15, 2017

Future::boxed and Stream::boxed should prevent double boxing #512

Future::boxed and Stream::boxed should prevent double boxing #512

Conversation

stepancheg commented Jun 11, 2017 • edited Loading

sinkuu commented Jun 11, 2017

3Hren Jun 11, 2017

Choose a reason for hiding this comment

stepancheg Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

3Hren Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

stepancheg Jun 11, 2017

Choose a reason for hiding this comment

stepancheg commented Jun 11, 2017

alexcrichton commented Jun 13, 2017

alexcrichton commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

stepancheg commented Jun 13, 2017

alexcrichton commented Jun 13, 2017

carllerche commented Jun 13, 2017

khuey commented Jun 17, 2017

alexcrichton commented Aug 15, 2017

stepancheg commented Jun 11, 2017 •

edited

Loading

stepancheg Jun 11, 2017 •

edited

Loading

3Hren Jun 11, 2017 •

edited

Loading